So, one of the things that I was in thoughts about was , if we should make this class simpler by making this entire map class synchronized (like ReferenceCountingMap in io..xds....security) instead of trying to deal with corner cases around atomic consistency which are difficult to understand and document.

Another thing worth considering is that potentially all operations on the map happen in syncContext since this map is only used when calling buildInterceptor which happens in syncContext , the release also either happens during similar control plane updates so synccontext.
It's possible for the cleanup to happen after an RPC end or close , but that's also covered since we do it inside synccontext using a RouteWrapper.
So, we may entirely not need this class to be thread safe.

So, I want opinions on where we should lean towards. IIUC, the ClusterRefState has semantics where it can be get from any thread and mutations from sync context.

Moving the synchronization down to the individual resource level and using a lock-free design for the hot path is superior. In a coarse-grained design where we do it at the SharedResourceManager (map) level, every single RPC across different ext-proc addresses would have to acquire a single global lock on SharedResourceManager during acquire and release.

It is not true that all operations on the map happen in syncContext. buildClientInterceptor is called when processing xDS updates on the Control plane in a syncContext. Not the SharedResourceManager.acquire and SharedResourceManager.release. acquire happen on the application's RPC thread and release on the transport thread that calls onClose when the rpc completes.

The individual resource is already synchronized to some degree, but could be potential leaks and stuff if acquire isn't synchronized(two races on map insertion), races between acquire and close , where acquire happens after closure. Solving these with atomics is very error prone.

I think all operations on the map happen in syncContext. the release happens via releasing the Route wrapper, which ensures releases happen via sync context.

The release can happen via the route wrapper on the control plane (syncContext) thread, but only if there are zero active RPCs at the moment the route is closed. If there are active RPCs, the cleanup tasks are deferred. The moment the last active RPC completes, its gRPC transport thread (onClose callback) is the thread that actually executes the route decrement and triggers the deferred cleanup tasks. Because SharedResourceManager is a global singleton across the channel, two different routes (Route A and Route B) can concurrently process RPCs that talk to the same ext_proc sidecar address. When Route A is draining and its last RPC completes, it calls release(address) on the transport thread. At the exact same time, Route B might be starting a new RPC and calling acquire(address) on an application thread.

So the need for synchronization during release is absolutely there. It is also there for acquire as I previously wrote that the acquire happens from the rpc application thread.

About simplifying the complex CAS (Compare And Swap) code, we could use synchronized for SharedResource retain and acquire. In SharedResourceManager in addition to the code being complex you are acquiring a lock on closeLock for any key whose SharedResource hasn't been created yet, so if there is a huge number of concurrent calls all for different keys for the first time there is unnecessary lock overhead. ConcurrentHashMap.compute() locks only the specific hash bucket of the key being queried. By using compute and computeIfPresent in acquire and release respectively, we can completely eliminate the global closeLock.

We had a bit of discussion last week,

• it should be possible for all accesses to the map to be made in sync context since map accesses only happen on control plan. There's a catch about closure which can happen on rpc end to release the route, but the route can hold a reference to sync context to ensure the closure happens in sync contxt. This would in theory eliminate the requirement of thread safety from the map itself.
• The resource however needs to be synchronized, since it'll get accessed from data plane rpcs. We brough forward dicussion about CAS in acquire but not in release, which was there because we validated count >0 in acquire , but did not have this validation in release . Updated release to have >0 validation and changed to CAS.
• Regardlng elimination of closeLock, this may not trivially possible without moving channel creation in the compute blocks, which may end up holding the lock longer depending on what the channel creation does.

Regardless, I believe the choice of how the map should behave would dictate a lot of implementation for the map

• if we choose to go non thread safe : We could remove all complexities (regular map and ints)
• if we choose to be threadsafe (or partially threadsafe like clusterRefState) : We could either choose between current implementation, using compute instead of closelocks or going full synchronized like ReferenceCountingMap.

I don't think the case is possible.

The only situation when the refcount from data plane rpc will be zero is when the route was already closed from the control plane. Since the route was closed no, more acquires can happen. Hence, race between acquire and release for refcount decrease to zero should not happen

But regardless, maybe it makes sense to be defensive and make the release follow the similar loop and pattern as retain by using computeIfAbsent or something.

I was talking about acquire for the same ext_proc address via a different route.

Ah. I see , that's a fair assessment.

We can technically solve this by using atomic eviction in release instead of a loop to bypass errors in acquire.

public boolean release(K key) {
    boolean[] closed = {false};
    resources.computeIfPresent(key, (k, shared) -> {
        if (shared.release()) {
            closed[0] = true;
            return null; // atomic eviction — entry never visible in dead state
        }
        return shared;
    });
    return closed[0];
}

Any preference between the two ?

Yes, this works and is preferable as it avoids the complex looping logic in acquire.

Done.